Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable ruff's flake8-return rule #3047

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

CoolCat467
Copy link
Member

In this pull request, we enable ruff's flake8-return rule and fix all the new issues from doing this.

Copy link

codecov bot commented Aug 1, 2024

Codecov Report

Attention: Patch coverage is 95.09804% with 5 lines in your changes missing coverage. Please review.

Project coverage is 99.61%. Comparing base (c4c8ce4) to head (e47c52e).

Files with missing lines Patch % Lines
src/trio/_core/_run.py 90.90% 1 Missing and 1 partial ⚠️
src/trio/_core/_io_kqueue.py 0.00% 1 Missing ⚠️
src/trio/_dtls.py 85.71% 1 Missing ⚠️
src/trio/_tests/test_subprocess.py 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3047      +/-   ##
==========================================
- Coverage   99.62%   99.61%   -0.02%     
==========================================
  Files         122      122              
  Lines       18405    18397       -8     
  Branches     1226     1227       +1     
==========================================
- Hits        18336    18326      -10     
- Misses         47       48       +1     
- Partials       22       23       +1     
Files with missing lines Coverage Δ
src/trio/_core/_mock_clock.py 100.00% <100.00%> (ø)
src/trio/_core/_tests/test_guest_mode.py 100.00% <100.00%> (ø)
src/trio/_core/_tests/test_run.py 100.00% <ø> (ø)
src/trio/_core/_tests/test_thread_cache.py 100.00% <100.00%> (ø)
src/trio/_core/_tests/test_windows.py 100.00% <100.00%> (ø)
src/trio/_core/_unbounded_queue.py 100.00% <100.00%> (ø)
src/trio/_file_io.py 100.00% <100.00%> (ø)
src/trio/_highlevel_generic.py 100.00% <100.00%> (ø)
src/trio/_highlevel_open_tcp_listeners.py 100.00% <100.00%> (ø)
src/trio/_highlevel_open_tcp_stream.py 100.00% <100.00%> (ø)
... and 22 more

@TeamSpen210
Copy link
Contributor

Not sure if this is worth it, it just causes a lot of churn indenting/dedenting things for no real reason. The elif technically is redundant if the previous if always returns, but it makes it more clear that they're mutually exclusive, and might cause bugs in future if someone removes one of the returns. This also causes symmetric if-else checks to become non-symmetric now, which might be confusing to the reader.

@CoolCat467 CoolCat467 added the skip newsfragment Newsfragment is not required label Aug 21, 2024
@A5rocks
Copy link
Contributor

A5rocks commented Aug 26, 2024

I agree with @TeamSpen210 that this doesn't really help anything. It doesn't make things clearer (unlike flake8-commas) and removes semantics. Looking through the error codes listed on https://pypi.org/project/flake8-return/ (I'm sure ruff implements them smarter, ofc):

  • R501 conflicts with mypy nvm, I was confusing this with implicit None return w/ a return type like None | int. I guess this is useful for standardization reasons, but I don't really like it.
  • R502/R503 is just type checking
  • R504 is potentially useful, but in general assigning to a variable is useful because it allows us to show a nicer name in tracebacks. (maybe we could just enable this? I'm not too sure)
  • R505/R506/R507/R508 remove semantics

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
skip newsfragment Newsfragment is not required
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants